Skip to content

aligns may include :center #1247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Dec 17, 2024
Merged

aligns may include :center #1247

merged 1 commit into from
Dec 17, 2024

Conversation

@soutaro
Copy link
Member

@soutaro soutaro commented Dec 17, 2024 •
edited
Loading

Context: rbs-gem ships with markdown documentations included in .rbs files, which is imported from RDoc. It uses RDoc::Markup::ToMarkdown formatter.

Problem: I recently found running our translator (rbs annotate) results in an error with Ruby v3_4_0_rc1 as follows:

/Users/soutaro/Developer/rdoc/lib/rdoc/markup/to_rdoc.rb:268:in `block in accept_table': nil is not a symbol nor a string (TypeError)

      h.__send__(a, w)
                 ^^^^
        from /Users/soutaro/Developer/rdoc/lib/rdoc/markup/to_rdoc.rb:267:in `map'
        from /Users/soutaro/Developer/rdoc/lib/rdoc/markup/to_rdoc.rb:267:in `accept_table'
        from /Users/soutaro/Developer/rdoc/lib/rdoc/markup/table.rb:29:in `accept'
        from /Users/soutaro/Developer/rdoc/lib/rdoc/markup/list_item.rb:53:in `block in accept'
        from /Users/soutaro/Developer/rdoc/lib/rdoc/markup/list_item.rb:52:in `each'
        from /Users/soutaro/Developer/rdoc/lib/rdoc/markup/list_item.rb:52:in `accept'
        from /Users/soutaro/Developer/rdoc/lib/rdoc/markup/list.rb:66:in `block in accept'
        from /Users/soutaro/Developer/rdoc/lib/rdoc/markup/list.rb:65:in `each'
        from /Users/soutaro/Developer/rdoc/lib/rdoc/markup/list.rb:65:in `accept'
        from /Users/soutaro/Developer/rdoc/lib/rdoc/markup/formatter.rb:75:in `block in accept_document'
        from /Users/soutaro/Developer/rdoc/lib/rdoc/markup/formatter.rb:70:in `each'
        from /Users/soutaro/Developer/rdoc/lib/rdoc/markup/formatter.rb:70:in `accept_document'
        from /Users/soutaro/Developer/rdoc/lib/rdoc/markup/document.rb:68:in `accept'
        from /Users/soutaro/Developer/rbs/lib/rbs/annotate/formatter.rb:87:in `translate'
        from ...

(The error is caused by a table included in Kernel.test documentation and other tables.)

Fix: I found that the aligns includes :center, not only nil. Adding a when clause fixes the problem.

I tried to walk through the test code, but couldn't find a good way to implement test for this. Can anyone help me?

@st0012 st0012 added the bug label Dec 17, 2024
st0012
st0012 approved these changes Dec 17, 2024
View reviewed changes
Copy link
Member

@st0012 st0012 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix. I can reproduce the error with tests but it requires updating multiple files. So I'll merge this first and update the tests in a separate PR 😄

PR: #1248

@st0012 st0012 merged commit cbbf04d into ruby:master Dec 17, 2024
22 checks passed
matzbot pushed a commit to ruby/ruby that referenced this pull request Dec 17, 2024
@soutaro @matzbot
[ruby/rdoc] aligns may include :center
408f536 
(ruby/rdoc#1247)

ruby/rdoc@cbbf04d6f8
st0012 added a commit that referenced this pull request Dec 17, 2024
@st0012
Update tests for #1247
40bde8b
st0012 added a commit that referenced this pull request Dec 17, 2024
@st0012
Update tests for #1247
0cd4354
st0012 added a commit that referenced this pull request Dec 17, 2024
@st0012
Update tests for #1247 (#1248)
94b9858
matzbot pushed a commit to ruby/ruby that referenced this pull request Dec 17, 2024
@st0012 @matzbot
[ruby/rdoc] Update tests for ruby/rdoc#1247
1254850 
(ruby/rdoc#1248)

ruby/rdoc@94b9858000
@soutaro soutaro deleted the table-align-center branch December 18, 2024 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@st0012 st0012 st0012 approved these changes

Assignees

No one assigned

Labels

bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@soutaro @st0012